I would be against having a random seed in tests unless the test ask for it. And even then, it would be better if the random value was printed somehow so that the test can be reproduced easily.

We discussed it with @alwa-nordic and it may be better to use the device ID (same as -d) as the random seed.

The harness should not set the random seed on its own, and less to an actual random value.

We discussed it with @alwa-nordic and it may be better to use the device ID (same as -d) as the random see

In the executable, the random seed already defaults to a value that depends on the device number. There is no need to do that again.

In the executable, the random seed already defaults to a value that depends on the device number. There is no need to do that again.

Okay, yeah I was pretty sure there was already something like that implemented but I wanted to test it today to confirm it ^^

I think it's more readable like that.

But I am wondering why you are looking at the change time? What is the reason?

Removing the files that were created by the test. Using generators with conditions is a standard practice in Python.

While you are right list comprehension are common practice, mixing the operations (filtering and the iteration) isn't a good practice.

You could do something like that if you want to keep the list comprehension:

It's a bit hard for me to understand why you are using eval() here. But this is something you should avoid if possible. It may execute arbitrary code.

@alwa-nordic is suggesting to use the str.format method instead.

I might have misunderstood what this does when I gave my suggestion. I assumed it's some kind of variable interpolation. Now I see it's for defining separate bsim_options per device, right?

I suggest we transpose bsim_exe_name, bsim_test_ids, and bsim_options in the config, so it looks like a script. What do you think? Example below:

devices:
    - exe: foo.exe
      bsim_testid: central
      custom_args:
        - -flash=bar.nvm.bin
        - -argstest
        - 10
    - exe: foo.exe
      bsim_testid: peripheral
      custom_args:
        - -flash=baz.nvm.bin
        - -argstest
        - 10

Not sure if returning the first element when the index is out of range is safe to do. Is it necessary? Seems like a bad practice that is hiding bugs.

Serves the purpose of handling the case (quite common one) when the same executable runs on all simulated devices.

It would be nicer to have something smarter than this. As an early stopgap it may be ok though.

What does this code do?

It's here to ensure that executable file needed for the test is compiled and ready.

So twister does not necessarily complete all build jobs before running? Won't this potentially suffer from a deadlock, e.g. when -j1?

If you for example run a test suite with one build test and a few run-only tests, and have 8 threads for this, run-only tests need this wait mechanism because they'll be attempted to launch at the same time as build test. With one thread, there will be no waiting in this function because when it's going to be called all binaries will be available.

There is no way we can have this.
a) we do not want actual randomness.
b) we have known issues that are triggered or not depending on the random seed, so this would mean CI would start failing at random in some runs. This is not something we can have.

Will remove it.

The harness should not set the random seed on its own, and less to an actual random value.

We discussed it with @alwa-nordic and it may be better to use the device ID (same as -d) as the random see

In the executable, the random seed already defaults to a value that depends on the device number. There is no need to do that again.

Do we need the - -?

Could RealEncryption have been it's own entry in yaml like real_encryption: true?

real_encryption: true

I would really not go that way. Which options you can pass to an executable is an unbound and unknown set. They depend on the test, configuration and target, and are not the problem of the twister harness.

Yes, it's a pretty long list of arguments which will easily double the size of the schema if we add all of them.

Doens't -argstest describe that the next arguments are test arguments? Where are the arguments here?

Looks like the common options are prepended to bsim_options below. I also think this is awkward.

Common section contains common options, allows to shorten the list when there is a lot of duplication so that it's possible to clearly see only the options which are different. However, that's not mandatory to use.

I think it's interesting that you have a test here that is a build-only and one that is a no-build.

What's preventing this from just being a test that is being built and run?

Each test here requires two different executables, one for scanner and one for advertising. Moving them to a separate build only test allows to build them only once and use the executables in subsequent tests without rebuilding.

There is a dependency between these tests that's not expressed here. The dut1 test leaves behind a file that is needed by the dut2 test.

I discussed this with @theob-pro. The way of least resistance is to make harness_config be a list of simulations that share simulation id and working directory and execute in sequence.

  bluetooth.host.id.settings:
    no_build: true
    harness_config:
      - bsim_test_ids:
          - dut1
      - bsim_test_ids:
          - dut2
        bsim_options:
          - -flash_rm

What do you think?

That's an interesting one. That really goes against the grain of testing when you make a test that relies on the result of a previous test. Ideally, it shouldn't be. So best solution is to redesign this test in my opinion, and not do anything like that again. As it stands now, the test will pass if it's run as a bunch (pipeline queue is lifo, hence reverse order here in testcase.yaml). What I really don't want to do, is to add this very special case to the framework.

I can say from the mesh perspective. The problem is that there is currently (unless I'm mistaken) no way to restart a bsim executable inside the test (basically the executable itself). For the test cases where we test that mesh correctly restores data from a persistent storage, we need to simulate a device reboot. And this is achieved by running several tests in a row using same simulation id:

That really goes against the grain of testing when you make a test that relies on the result of a previous test. Ideally, it shouldn't be. So best solution is to redesign this test in my opinion, and not do anything like that again

I think there is a bit of a misunderstanding here. These tests where something is run generating flash content, and then something else is run which uses that flash content, are not different tests. They are just sub-steps of the same test.
Meaning, many tests consists of just running a set of devices where all connect to one phy. But that is just the typical way, not the only way.
The tests @alwa-nordic refers to, run 2 or 3 sets of devices with phys in sequence (the whole sequence is one test). But we also have tests in tree running with the EDTT; and tests where only some of the devices executables connect to the Phy.
And that is in tree. Out of tree, we have tests that run postprocessing scripts on the simulation results and other programs to check what has happened.

One option would be to say that, by now, this twister harness does not support running those types of tests, but overall we need to keep supporting them.

I don't like that we are using ctime here. Won't this pick up files from parallel runs? Is there no better way to select files for cleaning?

If you use a naked except Exception: clause, you might end up catching exceptions other than the ones you expect to catch. This can hide bugs or make it harder to debug programs when unrelated errors are hidden.

https://pylint.readthedocs.io/en/v3.3.3/user_guide/messages/warning/broad-exception-caught.html

Understandable, but the purpose here is not to crash the whole session regardless of what happens, and use the status to just fail the test, and allow the session of move over to the next one.

Why does clean_exes call _set_start_time?

That's essentially the first actual harness function that is executed, at least for tests that are compiling.

It's unfortunate we cannot use the schema required field because the real type of harness_config is dependent on harness. What do you think about creating a bsim container field under harness_config?

Not very fond of it as well, but that's the current design, and I don't want to bloat the scope further.

What's the difference between self.instance.name and self.instance.platform.name?

The standard library has functools.cached_property that could be used here. I'll leave it up to you if you want to use it.

What's the goal of doing that? Why None?

Can be an empty string, doesn't matter. That's handling of a very common case when there is one binary file listed as 'bsim_exe_name' and doesn't have individual exes for bsim devices.